feat: Experimental implementation of custom modifiers #1991

kratsg · 2022-09-08T15:15:39Z

Pull Request Description

Refer to #850.

Checklist Before Requesting Reviewer

Tests are passing
"WIP" removed from the title of the pull request
Selected an Assignee for the PR to be responsible for the log summary

Before Merging

For the PR Assignees:

Summarize commit messages into a comprehensive review of the PR

* Add `pyhf.experimental` module to allow for user defined custom modifiers
  using numexpr string expressions.
* Add 'experimental' extra with numexpr dependency.
* Add tests for `pyhf.experimental.modifiers`.
* Add `pyhf.experimental` to API docs.
* Add 'experimental' extra to Docker image build.

codecov · 2022-09-08T15:26:09Z

Codecov Report

Attention: Patch coverage is 88.63636% with 10 lines in your changes missing coverage. Please review.

Project coverage is 98.09%. Comparing base (ecbe778) to head (bb302c2).
Report is 106 commits behind head on main.

Files with missing lines	Patch %	Lines
src/pyhf/experimental/modifiers.py	88.63%	6 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1991      +/-   ##
==========================================
- Coverage   98.28%   98.09%   -0.19%     
==========================================
  Files          69       70       +1     
  Lines        4538     4626      +88     
  Branches      803      814      +11     
==========================================
+ Hits         4460     4538      +78     
- Misses         45       51       +6     
- Partials       33       37       +4

Flag	Coverage Δ
contrib	`97.68% <88.63%> (-0.18%)`	⬇️
doctest	`61.13% <77.27%> (+0.42%)`	⬆️
unittests-3.10	`96.15% <88.63%> (-0.15%)`	⬇️
unittests-3.11	`96.15% <88.63%> (-0.15%)`	⬇️
unittests-3.8	`96.17% <88.63%> (-0.15%)`	⬇️
unittests-3.9	`96.19% <88.63%> (-0.15%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lukasheinrich · 2022-09-13T15:04:22Z

do we want to differentiate between experimental and contrib?

alexander-held · 2022-09-13T21:01:16Z

My reading of "contrib" is "quite stable API, but might get factored out eventually", while I would view "experimental" more like "anything goes, expect things to break". You could of course communicate how exactly you want the modules to be understood, but this is what I would think without further context.

matthewfeickert · 2022-09-13T21:14:08Z

do we want to differentiate between experimental and contrib?

Yes, I think that's a good idea if we really mean "experimental". I have the same take as @alexander-held does.

lukasheinrich · 2022-09-23T07:19:59Z

Ok !

alexander-held · 2022-12-20T12:51:50Z

Related to the discussion in scikit-hep/cabinetry#382, it may be good to provide a suitable schema for this as well (perhaps same PR makes sense?).

rmnmllr · 2022-12-22T15:00:15Z

If I may add a request, it would be great if pyhf could catch a wrongly typed function expression in the custom modifier. I stumbled over srqt instead of sqrt in the related cabinetry issue discussion scikit-hep/cabinetry#382 (comment) which produced an obscure error output.

matthewfeickert · 2023-01-25T17:39:32Z

@kratsg while I will go back and review Issue #850 (I've forgotten all of it) can you also comment on what else would be needed for this PR before it would be ready for review?

pariRieck · 2023-01-26T17:19:54Z

Hi, just to comment also here that on the side of an ATLAS analysis there is some urgency with this and a merge would help us.
We are looking for a way to incorporate different signal histograms into our statistical model and they scale differently with the parameter of interest, hence custom_modifiers are what we are looking for.
Apart from that, we should be fine.

jmontejo · 2023-03-27T12:28:49Z

Hi,
just wanted to add a +1 of an ATLAS analysis (RPV-multijets) depending on this to leave HistFitter for pyhf.

Cheers
Javier

jmontejo · 2023-03-27T12:32:38Z

Hi,
just wanted to add a +1 of an ATLAS analysis (RPV-multijets) depending on this to leave HistFitter for pyhf.

Cheers
Javier

for more information, see https://pre-commit.ci

matthewfeickert

@kratsg I assume that these prints were put in here for debugging? and that we can remove them and they aren't actually supposed to be in there for logging. Can you confirm?

matthewfeickert · 2023-10-10T19:46:08Z

src/pyhf/experimental/modifiers.py

+            self.builder_data[key][sample]["data"]["mask"] += moddata["mask"]
+            if thismod:
+                if thismod["name"] != func_name:
+                    print(thismod)


Suggested change

print(thismod)

I've removed the prints in bb302c2 but if we need them back for debugging quickly just rever it.

src/pyhf/experimental/modifiers.py

matthewfeickert · 2023-10-10T20:49:02Z

Relevant RTD build: https://pyhf--1991.org.readthedocs.build/en/1991/api.html#experimental

alexander-held · 2023-10-11T14:18:30Z

Small edge case: if you define a custom parameter to plug into add_custom_modifier and then define that parameter again in the "normal" way as a normfactor attached to a sample (like {"type": "normfactor", "name": "mu", "data": None} that seems to generally work but only as long as the bounds you put into the custom modifier version sync up with the bounds that the "normal" version would create by default. It probably would make sense to not add new bounds when the parameter is added as a normfactor.

alexander-held · 2023-10-13T08:40:10Z

While giving this another try, I ran into something that is either a bug or a usage question. I also have some API feedback. I opened #2350 for this to not clutter this PR too much.

kratsg mentioned this pull request Sep 8, 2022

Application of POIs using formulas #850

Open

matthewfeickert changed the base branch from master to main September 21, 2022 20:51

alexander-held mentioned this pull request Sep 23, 2022

docs: Add v0.7.0 release notes #1705

Merged

4 tasks

lukasheinrich mentioned this pull request Nov 15, 2022

closed form density functions as additive modifiers #2062

Open

1 task

matthewfeickert force-pushed the feat/addCustomModifierPython branch 2 times, most recently from 9d8d660 to 789097d Compare December 15, 2022 20:10

This was referenced Dec 20, 2022

feat: support models with custom modifiers scikit-hep/cabinetry#385

Open

Adding custom modifiers to cabinetry scikit-hep/cabinetry#382

Open

matthewfeickert force-pushed the feat/addCustomModifierPython branch from 789097d to c758f7e Compare December 27, 2022 23:49

matthewfeickert force-pushed the feat/addCustomModifierPython branch from c758f7e to 668073b Compare January 25, 2023 17:23

matthewfeickert force-pushed the feat/addCustomModifierPython branch from 668073b to 4bad866 Compare February 8, 2023 05:41

matthewfeickert force-pushed the feat/addCustomModifierPython branch from 6fab1ae to 4bf9844 Compare February 16, 2023 19:01

matthewfeickert force-pushed the feat/addCustomModifierPython branch from 4bf9844 to b857d9d Compare March 3, 2023 15:11

matthewfeickert force-pushed the feat/addCustomModifierPython branch from b857d9d to 55bc18f Compare April 7, 2023 06:06

matthewfeickert force-pushed the feat/addCustomModifierPython branch from 55bc18f to 1d0600e Compare April 19, 2023 16:33

matthewfeickert assigned kratsg Apr 19, 2023

kratsg and others added 3 commits October 10, 2023 01:09

add in the notebook from lukas

6a306d3

Add 'experimental' extra to pyproject.toml

dffe901

add is_shared

d572c67

matthewfeickert and others added 2 commits October 10, 2023 01:10

format

c1dcf81

[pre-commit.ci] auto fixes from pre-commit.com hooks

10be48a

for more information, see https://pre-commit.ci

matthewfeickert force-pushed the feat/addCustomModifierPython branch from a640c0b to 10be48a Compare October 10, 2023 06:11

matthewfeickert added feat/enhancement New feature or request API Changes the public API labels Oct 10, 2023

matthewfeickert added 8 commits October 10, 2023 13:25

Apply isort

f4f3c77

Make add_custom_modifier the only API and check if numexpr is installed

d2d3497

Add docstrints to experimental init

109a3c3

test experimental extras error if not installed

cfad34a

Simplify reload test

fabdf47

Apply suggestion from Sourcery

8e1c3da

split names with with _

c1a5f08

Install experimental extra in Docker image

520df69

matthewfeickert reviewed Oct 10, 2023

View reviewed changes

matthewfeickert added 2 commits October 10, 2023 14:53

split name on _ again

0bf30b8

docs: Add docstring for public method

d9ccf67

matthewfeickert added docs Documentation related tests pytest build Changes that affect the build system or external dependencies Docker Involving Docker images or builds labels Oct 10, 2023

matthewfeickert added 2 commits October 10, 2023 19:13

Add versionadded tag to docstring

5137845

remove print commands (REVERT IF NEEDED FOR TESTING)

bb302c2

kratsg added the experiment/belle2 Relevant to Belle-II's interests label Oct 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Experimental implementation of custom modifiers #1991

feat: Experimental implementation of custom modifiers #1991

kratsg commented Sep 8, 2022 •

edited by matthewfeickert

Loading

codecov bot commented Sep 8, 2022 •

edited

Loading

lukasheinrich commented Sep 13, 2022

alexander-held commented Sep 13, 2022

matthewfeickert commented Sep 13, 2022

lukasheinrich commented Sep 23, 2022

alexander-held commented Dec 20, 2022

rmnmllr commented Dec 22, 2022

matthewfeickert commented Jan 25, 2023

pariRieck commented Jan 26, 2023

jmontejo commented Mar 27, 2023

jmontejo commented Mar 27, 2023

matthewfeickert left a comment

matthewfeickert Oct 10, 2023

matthewfeickert Oct 11, 2023

matthewfeickert commented Oct 10, 2023

alexander-held commented Oct 11, 2023

alexander-held commented Oct 13, 2023

feat: Experimental implementation of custom modifiers #1991

Are you sure you want to change the base?

feat: Experimental implementation of custom modifiers #1991

Conversation

kratsg commented Sep 8, 2022 • edited by matthewfeickert Loading

Pull Request Description

Checklist Before Requesting Reviewer

Before Merging

codecov bot commented Sep 8, 2022 • edited Loading

Codecov Report

lukasheinrich commented Sep 13, 2022

alexander-held commented Sep 13, 2022

matthewfeickert commented Sep 13, 2022

lukasheinrich commented Sep 23, 2022

alexander-held commented Dec 20, 2022

rmnmllr commented Dec 22, 2022

matthewfeickert commented Jan 25, 2023

pariRieck commented Jan 26, 2023

jmontejo commented Mar 27, 2023

jmontejo commented Mar 27, 2023

matthewfeickert left a comment

Choose a reason for hiding this comment

matthewfeickert Oct 10, 2023

Choose a reason for hiding this comment

matthewfeickert Oct 11, 2023

Choose a reason for hiding this comment

matthewfeickert commented Oct 10, 2023

alexander-held commented Oct 11, 2023

alexander-held commented Oct 13, 2023

kratsg commented Sep 8, 2022 •

edited by matthewfeickert

Loading

codecov bot commented Sep 8, 2022 •

edited

Loading